Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use symbols for build status #8705

Merged

Conversation

janfaracik
Copy link
Contributor

Rather late followup to #7208 in which weather icons were replaced with symbols. This PR does the same for build status icons and also updates the appearance of some of them (do give feedback on this!).

The current approach to build status icons is a little unwieldy and can break in some scenarios (see below images). Using symbols is consistent with near enough every other icon in core Jenkins and keeps things simple to update in the future.

Before
image

After
image

Before (in Safari)
image

After
image

Testing done

  • Icons appear as expected on the dashboard
  • Icons appear as expected in build history (page and widget format)
  • Icons appear as expected in build time trend (fixes a UI issue in Safari too)

Proposed changelog entries

  • Refine build status icons

Proposed upgrade guidelines

N/A

Submitter checklist

Desired reviewers

@jenkinsci/sig-ux

Before the changes are marked as ready-for-merge:

Maintainer checklist

@yaroslavafenkin yaroslavafenkin added the security-approved @jenkinsci/core-security-review reviewed this PR for security issues label Nov 21, 2023
@timja
Copy link
Member

timja commented Nov 24, 2023

The alignment looks a little off in the build history?

Before:
image

After:

image

Apart from that looks better to me

@janfaracik
Copy link
Contributor Author

janfaracik commented Nov 24, 2023

The alignment looks a little off in the build history?

...

Apart from that looks better to me

Not seeing that on my end?

master
image

use-symbols-for-build-status
image

@timja
Copy link
Member

timja commented Nov 24, 2023

I see it in your screenshot to, maybe not required to fix it's just a bit higher than before.

@janfaracik
Copy link
Contributor Author

I see it in your screenshot to, maybe not required to fix it's just a bit higher than before.

Ah sorry I misunderstood - the icon position is fixed now.

Copy link
Member

@timja timja left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@timja timja added web-ui The PR includes WebUI changes which may need special expertise rfe For changelog: Minor enhancement. use `major-rfe` for changes to be highlighted labels Nov 24, 2023
@timja timja requested review from mawinter69 and a team November 24, 2023 22:15
@NotMyFault NotMyFault added the ath-successful This PR has successfully passed the full acceptance-test-harness suite label Nov 25, 2023
Copy link
Member

@NotMyFault NotMyFault left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great!

@timja
Copy link
Member

timja commented Nov 25, 2023

/label ready-for-merge


This PR is now ready for merge, after ~24 hours, we will merge it if there's no negative feedback.

Thanks!

@comment-ops-bot comment-ops-bot bot added the ready-for-merge The PR is ready to go, and it will be merged soon if there is no negative feedback label Nov 25, 2023
Copy link
Contributor

@mawinter69 mawinter69 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have to object. There are still several places where the old things are used. I even found a place where an old blue ball is used.
E.g. create a freestyle job that triggers another freestyle job. This lead to this:
image
Maybe that old blue ball is coming from https://plugins.jenkins.io/parameterized-trigger/

Places with old implementation:
lib/hudson/buildLink.jelly#55
hudon/model/AbstractBuild/index.jelly#79,#89
hudson/model/Run/statusIcon.jelly#34 (not sure if this is anywhere used
lib/hudson/jobLink.jelly#34

@timja timja removed the ready-for-merge The PR is ready to go, and it will be merged soon if there is no negative feedback label Nov 25, 2023
@mawinter69
Copy link
Contributor

Seems the blue ball was recently fixed: jenkinsci/parameterized-trigger-plugin#357

@janfaracik
Copy link
Contributor Author

janfaracik commented Nov 26, 2023

lib/hudson/buildLink.jelly#55
hudson/model/AbstractBuild/index.jelly#79,#89
hudson/model/Run/statusIcon.jelly#34
lib/hudson/jobLink.jelly#34

Updated those - thanks for finding.

@timja
Copy link
Member

timja commented Nov 26, 2023

/label ready-for-merge


This PR is now ready for merge, after ~24 hours, we will merge it if there's no negative feedback.

Thanks!

@comment-ops-bot comment-ops-bot bot added the ready-for-merge The PR is ready to go, and it will be merged soon if there is no negative feedback label Nov 26, 2023
@NotMyFault NotMyFault merged commit 1f95b09 into jenkinsci:master Nov 27, 2023
17 checks passed
@janfaracik janfaracik deleted the use-symbols-for-build-status-new branch November 27, 2023 20:49
@MarkEWaite
Copy link
Contributor

MarkEWaite commented Nov 29, 2023

@janfaracik thanks very much for the improvement in the build status display! I like it very much.

It appears that the improvement has exposed two unit tests in the JUnit plugin that were dependent on the previous layout. Would you be willing to look at those two tests to adapt them so that they will pass on both Jenkins 2.433 (before the change) and Jenkins 2.434 (after the change)?

Detected in plugin bill of materials pull request:

Raised as a JUnit plugin issue in:

If you're not able to investigate it, please let me know and I'll see if I can find someone else to help.

MarkEWaite added a commit to jenkinsci/bom that referenced this pull request Nov 29, 2023
jenkinsci/junit-plugin#588 is the issue report
for the JUnit plugin tests that depend on the icon based build status
display.

From jenkinsci/jenkins#8705 the Jenkins core
pull request that switched from using icon based build status to symbol
based build status.  That change also fixes a layout issue in the Safari
web browser.
renovate bot pushed a commit to jenkinsci/bom that referenced this pull request Nov 29, 2023
jenkinsci/junit-plugin#588 is the issue report
for the JUnit plugin tests that depend on the icon based build status
display.

From jenkinsci/jenkins#8705 the Jenkins core
pull request that switched from using icon based build status to symbol
based build status.  That change also fixes a layout issue in the Safari
web browser.
Comment on lines -70 to -102
function generateSVGIcon(iconName, iconSizeClass) {
const imagesURL = document.head.getAttribute("data-imagesurl");

const isInProgress = iconName.endsWith("anime");
let buildStatus = "never-built";
switch (iconName) {
case "red":
case "red-anime":
buildStatus = "last-failed";
break;
case "yellow":
case "yellow-anime":
buildStatus = "last-unstable";
break;
case "blue":
case "blue-anime":
buildStatus = "last-successful";
break;
case "grey":
case "grey-anime":
case "disabled":
case "disabled-anime":
buildStatus = "last-disabled";
break;
case "aborted":
case "aborted-anime":
buildStatus = "last-aborted";
break;
case "nobuilt":
case "nobuilt-anime":
buildStatus = "never-built";
break;
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Caused JENKINS-72407

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The problem is not here but in StatusColumn -> column.jelly

}
function generateSVGIcon(iconName) {
const icons = document.querySelector("#jenkins-build-status-icons");
iconName = iconName.replace("-anime", "");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

any reason why the anime svgs are excluded? Before they were animated when the build was running

MarkEWaite added a commit to jenkinsci/bom that referenced this pull request Dec 14, 2023
https://github.com/jenkinsci/junit-plugin/releases/tag/1252.vfc2e5efa_294f
is the JUnit plugin release that fixes the test.

Special thanks to Tim Jacomb for fixing the test in
jenkinsci/junit-plugin#591

jenkinsci/junit-plugin#588 is the issue report
for the JUnit plugin tests that depend on the icon based build status
display.

jenkinsci/jenkins#8705 is the Jenkins core
pull request that switched from using icon based build status to symbol
based build status.  That change also fixes a layout issue in the Safari
web browser.

This reverts commit cb2376e.

Author: Mark Waite <[email protected]>
Date:   Tue Nov 28 22:00:27 2023 -0700
github-actions bot pushed a commit to jenkinsci/bom that referenced this pull request Dec 14, 2023
…c2e5efa_294f in /bom-weekly (#2755)

* Bump org.jenkins-ci.plugins:junit in /bom-weekly

Bumps [org.jenkins-ci.plugins:junit](https://github.com/jenkinsci/junit-plugin) from 1240.vf9529b_881428 to 1252.vfc2e5efa_294f.
- [Release notes](https://github.com/jenkinsci/junit-plugin/releases)
- [Commits](https://github.com/jenkinsci/junit-plugin/commits)

---
updated-dependencies:
- dependency-name: org.jenkins-ci.plugins:junit
  dependency-type: direct:production
...

Signed-off-by: dependabot[bot] <[email protected]>

* Revert "Exclude AggregatedTestResultPublisherTest"

https://github.com/jenkinsci/junit-plugin/releases/tag/1252.vfc2e5efa_294f
is the JUnit plugin release that fixes the test.

Special thanks to Tim Jacomb for fixing the test in
jenkinsci/junit-plugin#591

jenkinsci/junit-plugin#588 is the issue report
for the JUnit plugin tests that depend on the icon based build status
display.

jenkinsci/jenkins#8705 is the Jenkins core
pull request that switched from using icon based build status to symbol
based build status.  That change also fixes a layout issue in the Safari
web browser.

This reverts commit cb2376e.

Author: Mark Waite <[email protected]>
Date:   Tue Nov 28 22:00:27 2023 -0700

---------

Signed-off-by: dependabot[bot] <[email protected]>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Co-authored-by: Mark Waite <[email protected]>
@@ -52,7 +52,7 @@ THE SOFTWARE.
</j:when>
<j:otherwise>
<a href="${attrs.href ?: rootURL+'/'+r.url}" class="model-link">
<l:icon alt="${r.iconColor.description}" class="${r.buildStatusIconClassName} icon-sm" style="margin-left: 0; position: relative; top: -0.1rem;"/><span class="jenkins-icon-adjacent">${jobName_}#<!-- -->${number}</span></a>
<l:icon alt="${r.iconColor.description}" src="symbol-status-${r.iconColor.iconName}" class="icon-sm" style="margin-left: 0; position: relative; top: -0.1rem;"/><span class="jenkins-icon-adjacent">${jobName_}#<!-- -->${number}</span></a>
Copy link
Member

@uhafner uhafner Dec 23, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This breaks the build link again (3rd time in the last past months) that is shown in the warnings and coverage links. Maybe we should provide a UI tests for this feature.

Bildschirmfoto 2023-12-23 um 17 16 32

@uhafner
Copy link
Member

uhafner commented Jan 4, 2024

@daniel-beck
Copy link
Member

Caused JENKINS-72711

@daniel-beck
Copy link
Member

Caused JENKINS-72845.

@alexsch01
Copy link

I made https://issues.jenkins.io/browse/JENKINS-73386
TLDR: I believe the aborted button and disabled button look too similar due to this PR

@daniel-beck
Copy link
Member

Caused JENKINS-74868.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ath-successful This PR has successfully passed the full acceptance-test-harness suite ready-for-merge The PR is ready to go, and it will be merged soon if there is no negative feedback rfe For changelog: Minor enhancement. use `major-rfe` for changes to be highlighted security-approved @jenkinsci/core-security-review reviewed this PR for security issues web-ui The PR includes WebUI changes which may need special expertise
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants